PR to analyze failures based on the Tigers PR. - #2038

tests/e2e/scripts/analyze_failures.sh (2)

56-72: Tighten large‑log preprocessing to avoid duplication and follow shell best practices

Two points worth adjusting here:

1. Avoid masking get_file_size exit status (SC2155)
   Declaring and assigning file_size in one line can hide a failure from get_file_size. Prefer:

-    local file_size=$(get_file_size "$log_file")
+    local file_size
+    file_size=$(get_file_size "$log_file")

2. Prevent double‑processing must‑gather logs
   The first find "${ARTIFACT_DIR}" ... walk will already include ${ARTIFACT_DIR}/must-gather/**, and then you loop over ${ARTIFACT_DIR}/must-gather again. That can duplicate work and inflate token usage for big runs. One simple fix is to exclude must-gather from the first search:

-        done < <(find "${ARTIFACT_DIR}" -maxdepth 4 -name "*.log" -type f 2>/dev/null | while read f; do
+        done < <(find "${ARTIFACT_DIR}" -maxdepth 4 -name "*.log" -type f ! -path "${ARTIFACT_DIR}/must-gather/*" 2>/dev/null | while read f; do

This keeps per‑test logs and must‑gather logs logically separated in the two loops and avoids redundant subagent calls.

Also applies to: 119-167

---

21-22: Make exit‑code handling and temp‑file cleanup more robust

The failure‑analysis behavior is correct, but a couple of small tweaks will harden the script:

1. Initialize and quote EXIT_CODE defensively

Right now EXIT_CODE=$1 and later if [ $EXIT_CODE -ne 0 ]; then and exit $EXIT_CODE. This is fine when the caller always passes a numeric code, but it is easy to make the script more bullet‑proof:

-EXIT_CODE=$1
+EXIT_CODE=${1:-0}
...
-if [ $EXIT_CODE -ne 0 ]; then
+if [ "$EXIT_CODE" -ne 0 ]; then
...
-exit $EXIT_CODE
+exit "$EXIT_CODE"

2. Use a quoted single‑quoted trap to avoid premature expansion (SC2064)

-    TEMP_OUTPUT=$(mktemp)
-    trap "rm -f $TEMP_OUTPUT" EXIT
+    TEMP_OUTPUT=$(mktemp)
+    trap 'rm -f "$TEMP_OUTPUT"' EXIT

This ensures the trap always refers to the actual temp file path and behaves correctly even if the variable were changed later.

Also applies to: 169-181, 188-193, 365-367, 372-373, 432-432

docs/design/claude-prow-failure-analysis_design.md (1)

629-648: Update test-e2e Makefile snippet to match the current implementation

The Makefile example here has diverged from the real test-e2e target (flag names, provider/credentials wiring, and CI‑only Claude setup). For someone copying this snippet, that mismatch will be confusing and could break local runs.

Recommend regenerating this section from the live Makefile (including the EXIT_CODE handling and analyze_failures.sh call) so the design doc remains an accurate reference.

build/ci-Dockerfile (1)

8-22: CI image wiring is correct; consider pinning Node/Claude versions

The changes correctly:

• Ensure tests/e2e/scripts/analyze_failures.sh is executable for CI.
• Switch kubectl installation to a multi‑arch flow using TARGETARCH.
• Add Node.js and the global @anthropic-ai/claude-code CLI needed by the analysis script.

To improve reproducibility of CI images, consider pinning:

• A specific Node.js major/minor (via a fixed NodeSource channel or explicit package version).
• A specific @anthropic-ai/claude-code version (e.g. npm install -g @anthropic-ai/claude-code@<version>).

That will make it easier to audit and debug CI behavior over time when dependencies evolve.

Makefile (1)

860-886: EXIT_CODE capture and Claude invocation look good; optionally initialize EXIT_CODE explicitly

The updated test-e2e target correctly:

• Runs ginkgo with the extended args (including $(HCP_EXTERNAL_ARGS)).
• Captures the test exit status into EXIT_CODE via ... || EXIT_CODE=$$?.
• In OpenShift CI, wires up Vertex AI env vars from /var/run/oadp-credentials/* and invokes analyze_failures.sh with the captured code.
• Exits with the original test result so Prow status is preserved.

For extra robustness against odd environments where EXIT_CODE might already be set, you could initialize it in this recipe shell before running ginkgo:

test-e2e: test-e2e-setup install-ginkgo
	EXIT_CODE=0; \
	ginkgo run ... || EXIT_CODE=$$?; \
	...
	exit $$EXIT_CODE

The current behavior is still correct in normal CI usage; this is just a defensive hardening.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting